Skip to content

Add IMPORT_ERRORS_ALL permission for import errors of files with no registered Dag#69790

Open
potiuk wants to merge 12 commits into
apache:mainfrom
potiuk:import-errors-all-permission-unregistered-files
Open

Add IMPORT_ERRORS_ALL permission for import errors of files with no registered Dag#69790
potiuk wants to merge 12 commits into
apache:mainfrom
potiuk:import-errors-all-permission-unregistered-files

Conversation

@potiuk

@potiuk potiuk commented Jul 12, 2026

Copy link
Copy Markdown
Member

Import errors are authorized per Dag defined in the file. Files with no registered Dag (parse failed before any Dag was defined, or all Dags removed) have no per-Dag key to authorize on. This adds a dedicated IMPORT_ERRORS_ALL view -- admin-granted by default, scoped per team via the file's bundle where the auth manager supports multi-team isolation -- and gates such files on it: callers without the permission are denied those import errors (403 on the single endpoint; excluded from the list endpoint) instead of receiving the raw stack trace.

Implements #67461.

  • New AccessView.IMPORT_ERRORS_ALL.
  • Auth managers gain an is_authorized_view_for_team method that defaults to is_authorized_view, so auth managers that do not implement it (including out-of-tree ones) keep working unchanged. The simple and Keycloak managers override it to enforce team scoping; the FAB manager grants the new permission to Admin by default.
  • Both Import Errors endpoints resolve the file's team from its bundle and authorize unregistered-file errors on the new view; the list endpoint excludes unauthorized rows from the results and total_entries so their existence does not surface.

Tests

  • test_import_error.py -- with the permission the raw stack trace is returned; without it the single endpoint returns 403 and the list endpoint omits the rows; team scoping is honored; no query-count regression.
  • test_base_auth_manager.py -- is_authorized_view_for_team falls back to is_authorized_view by default.
  • simple / FAB / Keycloak auth-manager suites.
Was generative AI tooling used to co-author this PR?
  • Yes -- Claude Opus 4.8 (1M context)

Generated-by: Claude Opus 4.8 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions

@potiuk

potiuk commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

@vincbeck — you'd offered to pick up #67461, so heads-up that I went ahead and implemented it here. Would you mind reviewing when you get a chance? Particularly keen on your read of the is_authorized_view_for_team addition (kept backwards-compatible so existing auth managers are untouched) and the deny-by-default behaviour for files with no registered Dag. Happy to adjust.

@henry3260 henry3260 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check bundle_name as well here — otherwise a same-named file in another bundle that does have Dags will match, and this unregistered file skips the IMPORT_ERRORS_ALL check.

Comment thread airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py Outdated
Comment thread airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py Outdated
@henry3260

Copy link
Copy Markdown
Contributor

I think we should check bundle_name as well here — otherwise a same-named file in another bundle that does have Dags will match, and this unregistered file skips the IMPORT_ERRORS_ALL check.

Maybe we can add a regression test here :)

Comment thread airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py Outdated
potiuk added 8 commits July 20, 2026 15:29
…egistered Dag

The Import Errors API authorizes each error per Dag defined in its file. Files
with no registered Dag have no per-Dag key to authorize on. Gate the raw stack
trace for those files on a dedicated, admin-by-default IMPORT_ERRORS_ALL view,
scoped per team via the file's bundle where the auth manager supports multi-team
isolation, and redact it for callers without the permission on both the list and
single endpoints.

Closes: apache#67461

Generated-by: Claude Opus 4.8 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
… view

The provider is released independently of core and may run against an older
Airflow that predates the AccessView.IMPORT_ERRORS_ALL member (added in 3.4.0).
Referencing it directly in the resource map raised AttributeError at import
time under the provider compatibility matrix. Resolve the member through a
common.compat shim and map it only when the running core defines it.
…lease

The IMPORT_ERRORS_ALL access-view shim is added to the common.compat provider in
this same PR, so fab and keycloak require the next common.compat release. The
selective-check common-compat guard enforces the '# use next version' marker on
those dependents; add it so the build-info check passes and the compatibility
tests run.
The same relative_fileloc can exist in more than one bundle, so matching only on
the filename treated an import error whose file has no Dag in its own bundle as
registered when another bundle happened to define a Dag for the same path. Join
the has-any-Dag subquery on both filename and bundle_name so unregistered-file
import errors are detected per bundle.
The shim module was added without a matching test, which the provider
project-structure check flags. Cover both branches: it resolves to the enum
member on a core that defines it, and to None on an older core that does not.
The list endpoint joins the has-any-Dag subquery twice; only the first was
scoped to the bundle. Since the subquery now yields one row per (file, bundle),
the remaining filename-only join could duplicate an error across bundles and
mis-detect registration. Join on bundle_name there too, add a regression test
for a file registered only in another bundle, and update the existing bundle
-join test, whose old expectation encoded the pre-fix cross-bundle leak.
The provider compatibility matrix runs this test against older cores that
predate AccessView.IMPORT_ERRORS_ALL (or lack api_fastapi entirely), where
referencing the member directly raised AttributeError. Compare the shim against
whatever the running core exposes (the member, or None) so it holds everywhere.
…point

The has-any-Dag lookup in get_import_error is scoped per bundle. Without
that scoping a same-named file registered in a different bundle resolved
as a Dag match, so the endpoint returned the raw stacktrace on the
strength of an unrelated bundle's read permission instead of gating on
IMPORT_ERRORS_ALL. The list endpoint already had this coverage.
@potiuk
potiuk force-pushed the import-errors-all-permission-unregistered-files branch from 56d620e to 7f0cf5d Compare July 20, 2026 13:47
Per review, add the optional team_name to is_authorized_view directly rather
than introducing a parallel is_authorized_view_for_team. Duplicating every
authorization method for its team-aware variant would bloat the auth manager
interface; the other is_authorized_* methods already carry team scoping through
their details object, and this keeps is_authorized_view consistent with them.

This is a breaking change for out-of-tree auth managers: an override that keeps
the old (access_view, user) signature imports cleanly but raises TypeError at
runtime the first time core authorizes a team-scoped view. Documented in a
significant newsfragment with the one-line fix.
@potiuk
potiuk requested a review from o-nikolas as a code owner July 21, 2026 11:59
@potiuk
potiuk requested a review from vincbeck July 21, 2026 12:01
…_view

Folding team_name into is_authorized_view made it a hard breaking change: an
out-of-tree auth manager, or a provider released before the argument existed,
keeps the old (access_view, user) signature and would raise TypeError the first
time core authorized a team-scoped view -- a runtime 500 with no import- or
startup-time signal.

Route team-scoped authorization through a new BaseAuthManager.authorize_view,
which detects (once, cached) whether the manager's override accepts team_name.
A legacy manager is called without it and keeps working; team scoping is ignored
so the view is authorized globally -- exactly what a manager with no multi-team
support does anyway -- and a RemovedInAirflow4Warning tells the operator that
some views (e.g. import errors for files with no registered Dag) are visible
across teams until the auth manager is upgraded. The public interface stays a
single is_authorized_view; the fallback is removed in Airflow 4.
potiuk added 2 commits July 21, 2026 14:32
This PR changes the common.compat provider alongside the amazon, fab and keycloak
providers. When common.compat changes with other providers in the same PR, each
provider depending on it must carry a '# use next version' comment so the release
manager bumps the constraint to the not-yet-released common.compat at release time.
fab and keycloak already had it; amazon was pulled into the changed set by the auth
manager update and was missing it, which the selective-checks Build info job flagged.
The team_name argument no longer forces a hard break — auth managers that
predate it keep working with a deprecation warning — so it is a feature rather
than a significant/breaking change. Replace the significant newsfragment with a
single feature note that states when team_name became available (Airflow 3.4.0)
and which bundled auth managers support it, and move the implementation detail
(the authorize_view fallback, the deprecation, how to make a manager team-aware)
into the auth manager documentation where implementers will look for it.

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I would trim down some of the very verbose comments. But non blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants